-
-
Notifications
You must be signed in to change notification settings - Fork 34.3k
inspector: fix compressed responses #61226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
|
Tagging you @legendecas because I saw some of your work on the inspector. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61226 +/- ##
========================================
Coverage 88.54% 88.55%
========================================
Files 704 704
Lines 208734 208865 +131
Branches 40271 40296 +25
========================================
+ Hits 184823 184950 +127
- Misses 15932 15948 +16
+ Partials 7979 7967 -12
🚀 New features to boost your workflow:
|
|
I just removed irrelevant code, improved testing/linting, built locally Node binary, and tested against my repro. Rebased first commit message following guidelines. Please let me know if something is missing since it's my first contribution in this repo. |
legendecas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, LGTM
| async function testGzipHttp() { | ||
| await testCompressedResponse(httpServer, 'gzip', '/gzip'); | ||
| } | ||
|
|
||
| async function testGzipHttps() { | ||
| await testCompressedResponse(httpsServer, 'gzip', '/gzip'); | ||
| } | ||
|
|
||
| async function testXGzipHttp() { | ||
| await testCompressedResponse(httpServer, 'x-gzip', '/x-gzip'); | ||
| } | ||
|
|
||
| async function testXGzipHttps() { | ||
| await testCompressedResponse(httpsServer, 'x-gzip', '/x-gzip'); | ||
| } | ||
|
|
||
| async function testDeflateHttp() { | ||
| await testCompressedResponse(httpServer, 'deflate', '/deflate'); | ||
| } | ||
|
|
||
| async function testDeflateHttps() { | ||
| await testCompressedResponse(httpsServer, 'deflate', '/deflate'); | ||
| } | ||
|
|
||
| async function testBrotliHttp() { | ||
| await testCompressedResponse(httpServer, 'br', '/br'); | ||
| } | ||
|
|
||
| async function testBrotliHttps() { | ||
| await testCompressedResponse(httpsServer, 'br', '/br'); | ||
| } | ||
|
|
||
| async function testZstdHttp() { | ||
| await testCompressedResponse(httpServer, 'zstd', '/zstd'); | ||
| } | ||
|
|
||
| async function testZstdHttps() { | ||
| await testCompressedResponse(httpsServer, 'zstd', '/zstd'); | ||
| } | ||
|
|
||
| async function testPlainHttp() { | ||
| await testCompressedResponse(httpServer, null, '/plain'); | ||
| } | ||
|
|
||
| async function testPlainHttps() { | ||
| await testCompressedResponse(httpsServer, null, '/plain'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it seems like these functions can be inlined as they are just one-liner.
|
|
||
| async function testPlainHttp2Secure() { | ||
| await testCompressedResponse(http2SecureServer, null, '/plain'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, these could be inlined.
fix: #61222
Using Chrome DevTools inspector tool, the response of the compressed requests is full of garbled symbols.
This PR implements proper compression decryption if the
content-encodingheader is returned in the response headers.Repro here: https://github.com/rubnogueira/nodejs-repro-61222